Open
Conversation
Contributor
📝 WalkthroughWalkthroughAdds plaintext message signing alongside existing sigHash signing: request type allows an optional Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant NostrProvider
participant ActionHandler as signSchnorrOrPrompt
participant Nostr
Client->>NostrProvider: hashAndSignSchnorr(plaintext)
activate NostrProvider
NostrProvider->>NostrProvider: enable()
NostrProvider->>ActionHandler: execute("signSchnorrOrPrompt", { message })
deactivate NostrProvider
activate ActionHandler
ActionHandler->>ActionHandler: Detect message mode (isMessageMode=true)
ActionHandler->>ActionHandler: Validate plaintext
ActionHandler->>ActionHandler: Check/request permissionMethod
alt Permission Granted
ActionHandler->>Nostr: hashAndSignSchnorr(plaintext)
activate Nostr
Nostr->>Nostr: TextEncoder -> bytes
Nostr->>Nostr: crypto.subtle.digest("SHA-256")
Nostr->>Nostr: schnorr.sign(hash, privateKey)
Nostr-->>ActionHandler: signature (hex)
deactivate Nostr
ActionHandler-->>NostrProvider: signature
else Permission Denied
ActionHandler-->>NostrProvider: error
end
deactivate ActionHandler
NostrProvider-->>Client: signature or error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app/screens/Nostr/ConfirmSignSchnorr.tsx (1)
97-102:⚠️ Potential issue | 🟠 MajorUse a prompt view that preserves the exact message.
ContentMessagewas fine for a fixed-length hash, but it is not safe for arbitrary plaintext here. Insrc/app/components/ContentMessage/index.tsx, Line 13 the body is clamped to 8 lines and only renders truthy content, so long messages can be truncated and empty messages disappear. It also does not preserve whitespace/newlines verbatim, which matters in a signing flow where users need to review the exact payload they are approving.A dedicated scrollable
pre/whitespace-pre-wrapview would be safer here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/screens/Nostr/ConfirmSignSchnorr.tsx` around lines 97 - 102, Replace the ContentMessage usage in ConfirmSignSchnorr.tsx with a dedicated preformatted prompt view that preserves the exact message and is scrollable: stop using ContentMessage (which clamps to 8 lines and drops falsy content in src/app/components/ContentMessage/index.tsx) and instead render the payload (plaintext || sigHash || "") inside a container that uses a <pre>-style rendering (CSS white-space: pre-wrap or pre; overflow: auto; allow text selection) so whitespace/newlines are preserved verbatim, long messages aren’t truncated, and empty strings still render visibly for user review.
🧹 Nitpick comments (1)
src/types.ts (1)
564-565: Make the Schnorr payload shape mutually exclusive.With both properties optional,
{}and{ sigHash, message }are both validMessageSignSchnorrpayloads. That pushes avoidable ambiguity intosignSchnorrOrPromptat runtime. An exclusive union keeps invalid request shapes out of the type system.💡 Possible typing update
export interface MessageSignSchnorr extends MessageDefault { - args: { - sigHash?: string; - message?: string; - }; + args: + | { + sigHash: string; + message?: never; + } + | { + message: string; + sigHash?: never; + }; action: "signSchnorr"; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/types.ts` around lines 564 - 565, The MessageSignSchnorr payload currently allows both sigHash and message to be optional which permits ambiguous shapes like {} or both fields present; update the MessageSignSchnorr type to an exclusive union so callers must provide either a sigHash or a message but not both (e.g., one branch declares sigHash: string with message?: never and the other declares message: string with sigHash?: never) so signSchnorrOrPrompt and related code can rely on a single valid shape at compile time.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/extension/background-script/actions/nostr/signSchnorrOrPrompt.ts`:
- Around line 26-35: The code uses !!plaintext to set isMessageMode which treats
an empty string as absent; instead detect presence of the field (e.g., check
whether "message" exists on message.args) to decide mode, then validate its type
only if provided. Update the logic in signSchnorrOrPrompt (look for plaintext,
isMessageMode, message.args.message and sigHash) so that isMessageMode =
("message" in message.args) or similar, and keep the existing type checks but
only run the string check for plaintext when the field is present; leave sigHash
validation for the hash mode unchanged.
---
Outside diff comments:
In `@src/app/screens/Nostr/ConfirmSignSchnorr.tsx`:
- Around line 97-102: Replace the ContentMessage usage in ConfirmSignSchnorr.tsx
with a dedicated preformatted prompt view that preserves the exact message and
is scrollable: stop using ContentMessage (which clamps to 8 lines and drops
falsy content in src/app/components/ContentMessage/index.tsx) and instead render
the payload (plaintext || sigHash || "") inside a container that uses a
<pre>-style rendering (CSS white-space: pre-wrap or pre; overflow: auto; allow
text selection) so whitespace/newlines are preserved verbatim, long messages
aren’t truncated, and empty strings still render visibly for user review.
---
Nitpick comments:
In `@src/types.ts`:
- Around line 564-565: The MessageSignSchnorr payload currently allows both
sigHash and message to be optional which permits ambiguous shapes like {} or
both fields present; update the MessageSignSchnorr type to an exclusive union so
callers must provide either a sigHash or a message but not both (e.g., one
branch declares sigHash: string with message?: never and the other declares
message: string with sigHash?: never) so signSchnorrOrPrompt and related code
can rely on a single valid shape at compile time.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fa3aa9d3-287a-4b94-a5dd-65ae573e2d28
📒 Files selected for processing (5)
src/app/screens/Nostr/ConfirmSignSchnorr.tsxsrc/extension/background-script/actions/nostr/signSchnorrOrPrompt.tssrc/extension/background-script/nostr/index.tssrc/extension/providers/nostr/index.tssrc/types.ts
src/extension/background-script/actions/nostr/signSchnorrOrPrompt.ts
Outdated
Show resolved
Hide resolved
rolznz
reviewed
Mar 11, 2026
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/extension/background-script/actions/nostr/signSchnorrOrPrompt.ts (1)
69-72:⚠️ Potential issue | 🟠 MajorAwait the bulk permission writes before returning.
forEach(async ...)does not wait foraddPermissionFor()to finish.DONT_ASK_ANYcan return before all permissions are persisted, and any rejection here will be unhandled. The user-visible failure mode is that prompts may still appear after selecting "don't ask any".💡 Suggested fix
if (promptResponse.data.permissionOption == DONT_ASK_ANY) { - Object.values(PermissionMethodNostr).forEach(async (permission) => { - await addPermissionFor(permission, host, promptResponse.data.blocked); - }); + await Promise.all( + Object.values(PermissionMethodNostr).map((permission) => + addPermissionFor(permission, host, promptResponse.data.blocked) + ) + ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/extension/background-script/actions/nostr/signSchnorrOrPrompt.ts` around lines 69 - 72, The current use of forEach with an async callback means addPermissionFor calls for DONT_ASK_ANY (PermissionMethodNostr) are not awaited and rejections are unhandled; change this to either map the PermissionMethodNostr values to an array of promises and await Promise.all(...) or use a for...of loop and await each addPermissionFor(permission, host, promptResponse.data.blocked) so that all permission writes complete (and any errors are caught/handled) before returning from signSchnorrOrPrompt.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/extension/background-script/actions/nostr/signSchnorrOrPrompt.ts`:
- Around line 69-72: The current use of forEach with an async callback means
addPermissionFor calls for DONT_ASK_ANY (PermissionMethodNostr) are not awaited
and rejections are unhandled; change this to either map the
PermissionMethodNostr values to an array of promises and await Promise.all(...)
or use a for...of loop and await each addPermissionFor(permission, host,
promptResponse.data.blocked) so that all permission writes complete (and any
errors are caught/handled) before returning from signSchnorrOrPrompt.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e1d3e47e-8f57-4e30-b8f7-a12b781c466c
📒 Files selected for processing (3)
src/extension/background-script/actions/nostr/signSchnorrOrPrompt.tssrc/extension/background-script/nostr/index.tssrc/extension/providers/nostr/index.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/extension/background-script/nostr/index.ts
- src/extension/providers/nostr/index.ts
pavanjoshi914
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
sign schnorr message function.
shows plaintext before signing in prompt
we have new function at provider level. but in backend we still use same signSchnorrOrPrompt furnction
if plaintext passed, hash and sign schnorr
Summary by CodeRabbit
New Features
Bug Fixes / Behavior